Skip to content

Conversation

lidiazuin
Copy link
Contributor

No description provided.

lidiazuin and others added 6 commits August 15, 2025 13:50
…neo4j#2490)

Since we moved them over from DBMS level (while still keeping the DBMS
level syntax, just as another syntax for `DATABASE *`)

---------

Co-authored-by: Mark Dixon <[email protected]>
Co-authored-by: Reneta Popova <[email protected]>
The Operations manual covers all releases of 2025.xx series. That's why
we cannot simply replace examples with new ones, as new functionality
has been added. We also need to retain examples for earlier versions of
Neo4j.
@NataliaIvakina
Copy link
Collaborator

NataliaIvakina commented Aug 28, 2025

On the page Database privileges, Figure 2. Syntax of GRANT and DENY database privileges is not updated.
Is this expected?

@NataliaIvakina
Copy link
Collaborator

NataliaIvakina commented Aug 28, 2025

On the page DBMS privileges, the Figure 1. Syntax of GRANT and DENY DBMS privileges is not updated.
The same question -- is this expected?

Copy link
Collaborator

@NataliaIvakina NataliaIvakina Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, the branch (on the right hand side) for SERVER MANAGEMENT -> SHOW SERVERS, EXECUTE Privileges, and SHOW SETTINGS privilege is unclear and might be wrong.
There are three equal groups:

  • SERVER MANAGEMENT (with SHOW SERVERS at the 2nd level),
  • EXECUTE PRIVILEGES (that include EXECUTE PROCEDURES, EXECUTE BOOSTED PROCEDURES, EXECUTE ADMIN(ISTRATOR) PROCEDURES, EXECUTE FUNCTIONS, and EXECUTE BOOSTED FUNCTIONS),
  • and SHOW SETTINGS.

I think, we should have three equal branches for them. At the moment, their hierarchy is unclear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The EXECUTE privileges aren't a group in the hierarchy: they don't have a common parent privilege.

They are included under ALL DBMS PRIVILEGES as that one includes EXECUTE FUNCTION, EXECUTE BOOSTED FUNCTION, EXECUTE PROCEDURE and EXECUTE BOOSTED PROCEDURE for all functions and procedures (which therefore also covers the EXECUTE ADMIN PROCEDURES as that one includes EXECUTE PROCEDURE and EXECUTE BOOSTED PROCEDURE but only for procedures marked as admin procedures). So the old image that currently exist in the documentation is correct to have them just on a line and not in a group, as the 'groups' are actual privileges themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if to make frames of the first level blocks bold? These blocks are:

  • ROLE MANAGEMENT
  • USER MANAGEMENT
  • IMPERSONATE
  • SERVER MANAGEMENT
  • DATABASE MANAGEMENT
  • ALIAS MANAGEMENT
  • PRIVILEGE MANAGEMENT
  • EXECUTE PRIVILEGES (I think they could be grouped together)
  • SHOW SETTINGS

@@ -76,7 +76,7 @@ For more details on the differences between graphs, databases, and the DBMS, ref

image::privileges_grant_and_deny_syntax_dbms_privileges.svg[width="800", title="Syntax of GRANT and DENY DBMS privileges"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image::privileges-grant-and-deny-syntax-dbms-privileges.svg[width="800", title="Syntax of GRANT and DENY DBMS privileges"]

@@ -480,7 +480,7 @@ GRANT [IMMUTABLE] TRANSACTION [MANAGEMENT] [( { * \| user[, ...] } )]
|===


image::privileges_grant_and_deny_syntax_database_privileges.svg[title="Syntax of GRANT and DENY Database Privileges"]
image::privileges-grant-and-deny-syntax-database-privileges.svg[title="Syntax of GRANT and DENY Database Privileges", role=popup]
Copy link
Collaborator

@NataliaIvakina NataliaIvakina Aug 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what is happening here. In line 187, image::privileges_hierarchy_database.svg[title="Database privileges hierarchy"] is used instead of the new file name with hyphens. However, the new image is rendered in the preview.

The file name is updated in the line 483, but the old image is rendered.

Copy link
Contributor

@Hunterness Hunterness Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the blocks are hard to read and the hierarchy structure is harder to see with this layout :(

(I assume this applies to more than just this one of the hierarchy pictures but I only opened this one so far)

Copy link
Contributor

@Hunterness Hunterness Sep 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks better in the preview than here on github though 🤷

Copy link
Collaborator

@renetapopova renetapopova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @lidiazuin. They all look good from a visual perspective but I've found some inconsistencies and things that might lead to misunderstandings.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alter database is missing
Screenshot 2025-09-08 at 13 04 42

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2541 (comment) I also noticed this :P

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-09-08 at 13 17 15

Copy link
Collaborator

@renetapopova renetapopova Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-09-08 at 13 19 37

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image I think some of your image comments doesn't load?

Copy link
Collaborator

@renetapopova renetapopova Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-09-08 at 13 31 37

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The connections that you noted that don't derive from the same place are all coming from ALL DBMS PRIVILEGES, that's the style of the arrows used in the technical diagrams. I can try to change the style, but just letting you know that this is the default styling.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so, but they look misleading, so I decided to tell you if you can change them somehow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no space between To role and [,...] in the original diagram. @Hunterness, is the space a problem?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters as such but generally I'd not have a space there if I write the command even if it parses (... TO role1, role2 vs ... TO role1 , role2) 🤷

I'll also say that if the other syntax images doesn't have the space I don't think we should have it here but be consistent about it

Copy link
Collaborator

@renetapopova renetapopova Sep 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-09-08 at 13 54 58

Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of current state

Status (to help me keep track): 3 open comments

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the database bubble looks empty, it should contain a lot of things not just alter database:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So most things seems to have been readded but ALTER DATABASE is still missing (between DROP DATABASE and SET DATABASE ACCESS)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing ALTER DATABASE (between DROP DATABASE and SET DATABASE ACCESS)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that this one have arrows, I'd like the other hierarchy ones to also have that

@neo4j-docops-agent
Copy link
Collaborator

Copy link
Contributor

@Hunterness Hunterness left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next round of comments, a lot of them on missing/to much bold :P

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TO in TO role[, ...] should not be bold...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I don't know if I like having the input parts being bold, the italics was more subtle when comparted to the regular text of the keywords. This is why I initially suggested that the keywords were put in bold to still have them be the stronger of the two, if that makes sense

Can you make the keywords bold? that could also separate the keywords and input if keywords are bold and input not

If we decide to switch so keywords are bold and input not, then all of my comments on which parts of which bubbles need to be updated to be bold/not-bold need to also be inverted as they follow the current version where input is bold

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The *|user[, ...] bubble after the transaction privileges should still have () as that is part of the syntax

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The *|user[, ...] bubble after IMPERSONATE should still have () as it's part of syntax

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TO in TO role[, ...] should not be bold...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pattern should be bold

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the TO in TO role[, ...] should not be bold

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name-globbing[, ...] should be bold

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before all the second level privileges came in the same/consistent order between the groups: create x, drop x, rename x, alter x, assign x, remove x, sub-management privilege, show x (of course only listing the sub-privileges they had)

But here the order isn't as consistent: take the role and user ones for example, they have SHOW in different places in their order

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sub-privileges of ALTER USER, it would be nice to have SET PASSWORD[S] and SET AUTH next to each other as they are related (both have to do with auth of the user)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants